src/components/canvas/tldrawStyles.ts (1)

77-90: Hardcoded color #dadddf won't adapt to dark themes.

The hover/active background color for node inspector menu items is hardcoded. If Roam supports dark mode (or may in the future), consider using a CSS variable or an inherited Blueprint token instead.

tsconfig.json (1)

29-29: Consider "react-jsx" for the automatic JSX transform.

With React 18, "react-jsx" eliminates the need for import React from 'react' in every JSX file. Since esbuild handles bundling independently, this primarily affects tsc type-checking, but switching would align with modern React conventions and reduce boilerplate imports.

src/utils/getBlockProps.ts (1)

9-23: Inner type guard in normalizeProps object branch is redundant.

Within the Object.entries mapping (lines 18-20), the typeof value === "object" && value !== null check duplicates what normalizeProps already handles at the top level. You can simplify by unconditionally recursing:

♻️ Simplify by always recursing on values

         Object.fromEntries(
             Object.entries(props).map(([key, value]) => [
               key.replace(/^:+/, ""),
-              typeof value === "object" && value !== null
-                ? normalizeProps(value as JsonValue)
-                : value,
+              normalizeProps(value as JsonValue),
             ]),
           )

scripts/compile.ts (2)

21-21: Unused variable envContents.

let envContents = null; is declared but never read or assigned elsewhere in this file. Remove it to avoid confusion.

🧹 Remove dead code

-let envContents = null;

---

85-90: external is required in the Zod schema but has no default.

If compile is called with a partial opts object (e.g., compile({ opts: { out: "foo" } })), Zod will throw because external is not optional and has no default. Consider adding .default([]) to make it safe for callers that don't need external mappings.

♻️ Add default for `external`

 const cliArgs = z.object({
   out: z.string().optional(),
   root: z.string().optional(),
   format: z.enum(["esm"]).optional(),
-  external: z.array(z.string()),
+  external: z.array(z.string()).default([]),
 });

scripts/build.ts (1)

3-26: Redundant double try/catch — build() already exits on failure.

build() catches errors and calls process.exit(1) on line 15, so main()'s outer catch (lines 22-24) is unreachable for any error originating from compile(). If the intent is defense-in-depth, consider removing the inner process.exit(1) and letting the error propagate to main(), or collapsing to a single level.

Simplified single-level alternative

-const build = async () => {
-  process.env = {
-    ...process.env,
-    NODE_ENV: process.env.NODE_ENV || "production",
-  };
-
-  console.log("Compiling ...");
-  try {
-    await compile({});
-    console.log("Compiling complete");
-  } catch (error) {
-    console.error("Build failed on compile:", error);
-    process.exit(1);
-  }
-};
-
-const main = async () => {
-  try {
-    await build();
-  } catch (error) {
-    console.error(error);
-    process.exit(1);
-  }
-};
+const main = async () => {
+  process.env.NODE_ENV = process.env.NODE_ENV || "production";
+  console.log("Compiling ...");
+  try {
+    await compile({});
+    console.log("Compiling complete");
+  } catch (error) {
+    console.error("Build failed on compile:", error);
+    process.exit(1);
+  }
+};

.github/workflows/main.yaml (1)

24-30: Consider omitting version: 9 and letting pnpm/action-setup read from packageManager.

package.json pins "packageManager": "pnpm@9.15.0", but the workflow specifies version: 9, which may install a different 9.x minor. pnpm/action-setup@v4 can auto-detect the version from the packageManager field if version is omitted, ensuring CI and local dev use the exact same pnpm version.

Proposed fix

       - uses: pnpm/action-setup@v4
-        with:
-          version: 9
       - uses: actions/setup-node@v4
         with:
           node-version: "20"
           cache: "pnpm"

src/index.ts (1)

68-70: TODO: Migration from roamjs-query-builder.tldraw.

Flagging the TODO on line 68. This notes a pending migration path for users coming from the older roamjs-query-builder.tldraw data format.

Would you like me to open an issue to track this migration task?

package.json (1)

35-35: Update esbuild from 0.17.14 to a more recent version.

The pinned version is nearly 3 years old. Unlike suggested in the original note, roamjs-components@0.86.4 does not list esbuild as a dependency, so this pinning is not required for compatibility. Newer versions offer significant performance improvements and bug fixes—current stable version is 0.27.3.

src/components/canvas/uiOverrides.tsx (1)

109-124: Inline component ViewMenu defined inside MainMenu render body causes remounts.

ViewMenu is declared as a new function component on every render of MainMenu. React sees a new component type each time, so it will unmount and remount the subtree on every render. Hoist it outside the MainMenu factory or memoize it.

Proposed fix — hoist ViewMenu

+const ViewMenu = () => {
+  const actions = useActions();
+  return (
+    <TldrawUiMenuSubmenu id="view" label="menu.view">
+      <TldrawUiMenuGroup id="view-actions">
+        <TldrawUiMenuItem {...actions["zoom-in"]} />
+        <TldrawUiMenuItem {...actions["zoom-out"]} />
+        <ZoomTo100MenuItem />
+        <ZoomToFitMenuItem />
+        <ZoomToSelectionMenuItem />
+        <TldrawUiMenuItem {...actions["toggle-full-screen"]} />
+      </TldrawUiMenuGroup>
+    </TldrawUiMenuSubmenu>
+  );
+};
+
 export const createUiComponents = (): TLUiComponents => ({
   ...
   MainMenu: () => {
-    const ViewMenu = () => {
-      const actions = useActions();
-      return (
-        <TldrawUiMenuSubmenu id="view" label="menu.view">
-          <TldrawUiMenuGroup id="view-actions">
-            <TldrawUiMenuItem {...actions["zoom-in"]} />
-            <TldrawUiMenuItem {...actions["zoom-out"]} />
-            <ZoomTo100MenuItem />
-            <ZoomToFitMenuItem />
-            <ZoomToSelectionMenuItem />
-            <TldrawUiMenuItem {...actions["toggle-full-screen"]} />
-          </TldrawUiMenuGroup>
-        </TldrawUiMenuSubmenu>
-      );
-    };
     return (
       <DefaultMainMenu>
         <EditSubmenu />
         <ViewMenu />

src/components/canvas/Tldraw.tsx (3)

458-492: editor.on("event", ...) fires on every editor event — potential performance concern and fragile cast.

refreshInspectorTarget() is invoked on every single editor event (pointer moves, wheel, keyboard, etc.), each time calling editor.getOnlySelectedShape(). This is wasteful when only selection changes matter. The cast event as TLPointerEventInfo on line 459 is also unsafe for non-pointer events (fields like shiftKey/ctrlKey will be undefined), though the e.name === "pointer_up" guard prevents misuse.

Consider listening to selection changes specifically (e.g., editor.store.listen filtered to selection changes or editor.on("change", ...) with appropriate filtering) instead of all events.

---

408-520: Large onMount callback — consider extracting editor setup logic.

The onMount handler spans ~110 lines, registering an event listener, defining refreshInspectorTarget, and registering an external content handler. Extracting these into named setup functions would improve readability and testability.

---

59-67: getPageUidByPageTitle(title) called on every render.

This synchronous Roam API call runs unconditionally on each render. If the function has any cost, wrap it in useMemo keyed on title.

Proposed fix

-  const pageUid = getPageUidByPageTitle(title);
+  const pageUid = useMemo(() => getPageUidByPageTitle(title), [title]);

src/utils/isCanvasPage.ts (1)

15-18: Wildcard * maps to .+ (one-or-more) — is zero-or-more intended?

The * wildcard is replaced with .+, requiring at least one character. In typical glob semantics, * matches zero or more characters (.*). With the current implementation, a pattern Canvas/* won't match a page titled exactly Canvas/ (trailing slash, no suffix). This may be intentional but is worth confirming.

Regarding the static analysis ReDoS warning: the input is fully escaped before the \* → .+ substitution, so user-supplied patterns cannot inject arbitrary regex. The warning is a false positive here.

src/components/canvas/useRoamStore.ts (1)

95-111: useMemo performs side effects (loadSnapshot).

useMemo is intended for pure computation. The loadSnapshot call mutates the store, which is a side effect. React may re-run useMemo in future concurrent mode without guarantees. This is a common pattern in tldraw integrations, but be aware that React Strict Mode in development will double-invoke this, potentially loading the snapshot twice.

src/components/canvas/DefaultNodeUtil.tsx (1)

57-58: Duplicate escapeRegex function — also defined in isCanvasPage.ts.

This file's version additionally escapes double quotes (for Datomic embedding). Consider extracting a shared base escapeRegex and composing the Datomic-specific variant to avoid duplication.

package.json (1)

39-43: Exact-pinned peer dependencies are unusual and may cause installation warnings.

Peer dependencies are typically specified as ranges (e.g., "react": "^18.2.0") to allow flexibility for the consuming environment. Exact pins ("18.2.0") will emit warnings or errors if the host has even a patch-level difference (e.g., 18.2.1). Since this is a Roam Research extension where the host provides React, consider using >=18.2.0 <19 or similar ranges unless exact versions are strictly required.

Suggested change

   "peerDependencies": {
-    "react": "18.2.0",
-    "react-dom": "18.2.0",
-    "@blueprintjs/core": "3.50.4"
+    "react": "^18.2.0",
+    "react-dom": "^18.2.0",
+    "@blueprintjs/core": "^3.50.4"
   },

src/components/canvas/uiOverrides.tsx (1)

118-143: ViewMenu is redefined on every render of MainMenu, causing unnecessary unmount/remount cycles.

Declaring ViewMenu as a component inside the MainMenu render body means React sees a new component type on each render, discarding the subtree and recreating it (losing internal state and triggering extra DOM work). Hoist it outside createUiComponents or at least make it a stable reference.

♻️ Proposed fix — hoist ViewMenu

+const ViewMenu = () => {
+  const actions = useActions();
+  return (
+    <TldrawUiMenuSubmenu id="view" label="menu.view">
+      <TldrawUiMenuGroup id="view-actions">
+        <TldrawUiMenuItem {...actions["zoom-in"]} />
+        <TldrawUiMenuItem {...actions["zoom-out"]} />
+        <ZoomTo100MenuItem />
+        <ZoomToFitMenuItem />
+        <ZoomToSelectionMenuItem />
+        <TldrawUiMenuItem {...actions["toggle-full-screen"]} />
+      </TldrawUiMenuGroup>
+    </TldrawUiMenuSubmenu>
+  );
+};
+
 export const createUiComponents = (): TLUiComponents => ({
   // ...
   MainMenu: () => {
-    const ViewMenu = () => {
-      const actions = useActions();
-      return (
-        <TldrawUiMenuSubmenu id="view" label="menu.view">
-          <TldrawUiMenuGroup id="view-actions">
-            <TldrawUiMenuItem {...actions["zoom-in"]} />
-            <TldrawUiMenuItem {...actions["zoom-out"]} />
-            <ZoomTo100MenuItem />
-            <ZoomToFitMenuItem />
-            <ZoomToSelectionMenuItem />
-            <TldrawUiMenuItem {...actions["toggle-full-screen"]} />
-          </TldrawUiMenuGroup>
-        </TldrawUiMenuSubmenu>
-      );
-    };
     return (
       <DefaultMainMenu>
         <EditSubmenu />
         <ViewMenu />
         <ExportFileContentSubMenu />
         <ExtrasGroup />
         <PreferencesGroup />
       </DefaultMainMenu>
     );
   },
 });

src/components/canvas/Tldraw.tsx (4)

159-162: Stale toggleMaximized closure captured by useMemo — safe today, fragile under refactoring.

toggleMaximized is recreated every render but useMemo on line 159 only re-runs when maximizeKbd changes. This works today because toggleMaximized only reads refs (containerRef, appRef), which are always current. However, if someone later adds state or prop dependencies to toggleMaximized, the memoized override will silently use the stale version.

Consider wrapping toggleMaximized in useCallback (with [] deps since it only touches refs) and including it in the useMemo dep array, or adding a comment noting the ref-only constraint.

---

223-231: Including selectedUid in the dependency array of its own setter effect is a code smell.

The effect that synchronizes selectedUid with visibleResults lists selectedUid itself as a dependency. It works because React bails out on setting the same value, but it causes the effect to re-run every time selectedUid changes (including from keyboard navigation in moveSelection), doing a redundant scan of visibleResults. Prefer a functional update or remove selectedUid from the dep list.

♻️ Proposed fix — drop selectedUid from deps

  useEffect(() => {
    if (!visibleResults.length) {
      setSelectedUid("");
      return;
    }
-   if (!selectedUid || !visibleResults.some((r) => r.uid === selectedUid)) {
-     setSelectedUid(visibleResults[0].uid);
-   }
- }, [visibleResults, selectedUid]);
+   setSelectedUid((prev) =>
+     prev && visibleResults.some((r) => r.uid === prev) ? prev : visibleResults[0].uid,
+   );
+ }, [visibleResults]);

---

456-490: Event listener on editor.on("event", ...) fires refreshInspectorTarget on every editor event — consider filtering.

refreshInspectorTarget() (line 458) runs on every editor event (pointer move, wheel, key, etc.), each time calling editor.getOnlySelectedShape() and running through the setter logic. For high-frequency events like pointer moves, this adds overhead. Consider limiting the listener to selection-relevant events (e.g., "change" or specific event names) or debouncing the refresh.

---

307-337: useEffect with [] deps captures handleDropPayload closure — correct but non-obvious.

The empty dependency array means onDropNative captures the initial render's handleDropPayload. This works only because handleDropPayload reads appRef.current (a ref, always current) and calls stable imported functions. A comment explaining this invariant would help future maintainers, or alternatively wrap handleDropPayload in useCallback.

src/components/canvas/DefaultNodeUtil.tsx (1)

271-301: Dynamically generated StateNode classes share a single class identity per call — verify tldraw handles this correctly.

createDefaultNodeShapeTools uses .map() to generate two classes both named DefaultNodeTool (the class declaration name), differentiated only by the static id override. Since the consumer in Tldraw.tsx wraps this in useMemo([], []), the classes are stable across renders. However, the shared class name may cause confusion in React DevTools or error stack traces.

♻️ Optional: give each class a distinct name

 export const createDefaultNodeShapeTools = (): TLStateNodeConstructor[] =>
   DEFAULT_NODE_TOOLS.map(
     ({ id }) => {
-      class DefaultNodeTool extends StateNode {
+      const Tool = class extends StateNode {
         static override id = id;
         static override initial = "idle";
         shapeType = id;
         // ...
-      }
-      return DefaultNodeTool;
+      };
+      Object.defineProperty(Tool, "name", { value: `${id}Tool` });
+      return Tool;
     },
   );